Add loadbalancer support by ehelms · Pull Request #46 · Katello/smart_proxy_container_gateway

ehelms · 2025-01-09T20:51:34Z

When deploying the container gateway behind a load-balancer, the client is given a redirect to the location of the container image as reported by Pulp. This location known by Pulp contains the URL of the smart-proxy and has no knowledge of the load-balancer to send a proper redirect.

This changes introduces a new setting to specification of a loadbalancer when one is in use. If this is provided then the redirect will use that value to calculate the right URL to send back to the client for it to fetch manifests and blobs.

Could we have just used the existing pulp_endpoint setting? No. That setting is used by the container gateway itself to make requests to Pulp and if we set it to the load-balancer than all "internal" requests would route all the way out to and back through the load-balancer unnecessarily.

There is some duplicated code between the manifest endpoint and blob handling I intend to look at re-factoring.

The puppet class handling the container gateway, https://github.com/theforeman/puppet-foreman_proxy/blob/master/manifests/plugin/container_gateway.pp, would need to set this using the foreman_proxy::registration_url that we currently have users set when using a loadbalancer.

ekohl · 2025-02-28T10:28:06Z

I'm trying to capture our offline discussion here.

Essentially the problem is that we have a control plane (Foreman <-> Foreman Proxy/Pulp) and a user facing endpoint (User -> Pulp). It was designed that there was just a single URL, which is a false assumption.

I'd like to have a more generic naming: load balancer implies a certain implementation, but the specific implementation actually wasn't behind a load balancer. Instead it was in an Amazon VPC so it was closer to multi homing.

There was also a thought to reuse settings from smart_proxy_pulp but that currently doesn't have the split either so it wouldn't help.

ianballou · 2025-03-03T19:23:34Z

lib/smart_proxy_container_gateway/container_gateway.rb

          Proxy::ContainerGateway::ContainerGatewayMain.new(
            database: container_instance.get_dependency(:database_impl),
-            **settings.slice(:pulp_endpoint, :pulp_client_ssl_ca, :pulp_client_ssl_cert, :pulp_client_ssl_key)
+            **settings.slice(:pulp_endpoint, :pulp_client_ssl_ca, :pulp_client_ssl_cert, :pulp_client_ssl_key, :loadbalancer)


Since the config option naming came up:

Suggested change

**settings.slice(:pulp_endpoint, :pulp_client_ssl_ca, :pulp_client_ssl_cert, :pulp_client_ssl_key, :loadbalancer)

**settings.slice(:pulp_endpoint, :pulp_client_ssl_ca, :pulp_client_ssl_cert, :pulp_client_ssl_key, :client_content_endpoint)

This configurable option determines where we redirect the client to to get their requested content. So, I propose that the generic name be client_content_endpoint.

If that's too generic we could do something like container_content_endpoint. Or maybe container_delivery_endpoint?

ianballou · 2025-03-03T19:28:03Z

I'm trying to capture our offline discussion here.

Essentially the problem is that we have a control plane (Foreman <-> Foreman Proxy/Pulp) and a user facing endpoint (User -> Pulp). It was designed that there was just a single URL, which is a false assumption.

I'd like to have a more generic naming: load balancer implies a certain implementation, but the specific implementation actually wasn't behind a load balancer. Instead it was in an Amazon VPC so it was closer to multi homing.

There was also a thought to reuse settings from smart_proxy_pulp but that currently doesn't have the split either so it wouldn't help.

@ekohl this makes it sound like the current approach is generally accepted with some small modifications. If that's the case, I was thinking of creating a community post soon with the plan here for visibility and comments.

ekohl · 2025-03-03T21:04:51Z

@ekohl this makes it sound like the current approach is generally accepted with some small modifications. If that's the case, I was thinking of creating a community post soon with the plan here for visibility and comments.

The pattern exists in multiple places. In the Smart Proxy we have the templates URL, but also the registration URL. There it exists for similar reasons (control plane + client facing URL).

It would be nice if we could somehow standardize it so there's less configuration required but until then ...

ianballou · 2025-03-05T15:01:18Z

From some outside chats, it sounds like I'm going to take this PR over from here.

ehelms · 2025-03-07T01:44:52Z

The pattern exists in multiple places. In the Smart Proxy we have the templates URL, but also the registration URL. There it exists for similar reasons (control plane + client facing URL).

It would be nice if we could somehow standardize it so there's less configuration required but until then ...

Are you thinking a core smart_proxy settings value that represent the client facing URL that plugins can then use?

ekohl · 2025-03-07T09:11:29Z

I would be tempted to. This particular case feels a bit more tricky because it's the Pulp URL, not Smart Proxy

ehelms · 2025-03-10T18:30:43Z

The current state of the art is that we define a client_facing_servername built from the --foreman-proxy-registration-url parameter. We then use that value to define a URLs / settings:

(registration_url)[https://github.com/theforeman/puppet-foreman_proxy/blob/48fc520f2cd5a4e14908293d9e72a8154c79a9cd/manifests/module/registration.pp#L15]
(pulpcore_content_url)[https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/manifests/init.pp#L304]
(rhsm_hostname)[https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/manifests/init.pp#L311]

So we are saying currently, if you want to set a loadbalancer, or a VPC, set the registration URL and then everything else will inherit this indirectly to configure client facing settings to the loadbalancer.

That could be good enough reason to define a smart_proxy setting that is client_url or client_hostname. Or for now, we have this plugin define a client_endpoint and set the value the same way described above.

ianballou · 2025-03-13T19:15:56Z

The current state of the art is that we define a client_facing_servername built from the --foreman-proxy-registration-url parameter. We then use that value to define a URLs / settings:

(registration_url)[https://github.com/theforeman/puppet-foreman_proxy/blob/48fc520f2cd5a4e14908293d9e72a8154c79a9cd/manifests/module/registration.pp#L15] (pulpcore_content_url)[https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/manifests/init.pp#L304] (rhsm_hostname)[https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/manifests/init.pp#L311]

So we are saying currently, if you want to set a loadbalancer, or a VPC, set the registration URL and then everything else will inherit this indirectly to configure client facing settings to the loadbalancer.

That could be good enough reason to define a smart_proxy setting that is client_url or client_hostname. Or for now, we have this plugin define a client_endpoint and set the value the same way described above.

Is it safe to say then that this new loadbalancer setting in the current working implementation is exactly the same as $client_facing_servername in the Puppet modules?

If the new setting is something generic like client_endpoint, I think it wouldn't hurt to have it defined in the main smart_proxy project.

If that is the case, then we could:

Create the client_endpoint config option in smart_proxy
Use that config option in this PR
Update puppet-foreman_proxy (or puppet-foreman_proxy_content?) to take client_facing_servername and put it in the smart_proxy config file.

Hopefully it's okay to assume that users have --foreman-proxy-registration-url set to something sane, otherwise the container_gateway could be redirecting them somewhere strange. Hopefully it's only used for the loadbalancing scenario?

ehelms · 2025-03-13T20:32:08Z

@ekohl Based on these latest comments, what is your opinion on where the setting should live?

ehelms · 2025-03-20T19:29:23Z

@ianballou let's just go with a local setting for now, we can re-think it in the future if a core setting that is useful comes along

ianballou · 2025-03-20T22:08:21Z

That sounds good to me. I'll wrap this up then.

ianballou · 2025-03-21T20:17:33Z

@ehelms mind linking up https://projects.theforeman.org/issues/38304 to this PR ?

Signed-off-by: Eric D. Helms <ericdhelms@gmail.com>

ianballou

This is looking good to me! I gave it a test on a normal proxy by setting the new client_endpoint option. It's clearly trying to fetch the content from that endpoint.

I just want some confirmation first about it working okay on an actual load balancer.

If I change the host to something fake and look at the error, there is a token included in the URL, which I wonder about how it could apply to a second Pulp instance.

Example:

Get "https://centos10-proxy-devel.manicotto.example.com/pulp/container/default_organization/precipitation/containers/buttermilk_biscuits/prometheus_busybox/manifests/latest?expires=1742593318&validate_token=6c493ecf8fba8e1d2198670b7d478580e54e63a91185f92638d8e31235aabf53:e1a0ffbf8d234dd84c52be18f1f80f4432a0c2648a0f7e7ddfde18ad767c040c":

ianballou

Tested on a load balancer setup:

[~]# podman pull loadbalancer.example.com/default_organization/small_containers/prometheus_busybox
Trying to pull loadbalancer.example.com/default_organization/small_containers/prometheus_busybox:latest...
Getting image source signatures
Copying blob 1617e25568b2 done   | 
Copying blob 9fa9226be034 done   | 
Copying config 56a9b5a744 done   | 
Writing manifest to image destination
56a9b5a744a674481a5c034fec45ebf470a58ab844cd0da4d26b89bd27df5e36
[~]# podman image ls
REPOSITORY                                                                                                TAG         IMAGE ID      CREATED        SIZE
loadbalancer.example.com/default_organization/small_containers/prometheus_busybox  latest      56a9b5a744a6  15 months ago  3.92 MB

Also tested that a bogus client_endpoint would not result in pulled content as a sanity-check.

Finally, checked that the container gateway still functioned (although did not redirect to the load balancer) without error after removing the client_endpoint setting.

pr-processor bot added Not yet reviewed Waiting on contributor labels Jan 9, 2025

ehelms force-pushed the add-loadbalancer-support branch 2 times, most recently from 68e9286 to 1b02464 Compare January 14, 2025 16:12

ianballou self-requested a review January 28, 2025 18:46

ianballou reviewed Mar 3, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Mar 3, 2025

ianballou marked this pull request as ready for review March 10, 2025 18:25

ehelms force-pushed the add-loadbalancer-support branch from 1b02464 to f13fc6f Compare March 21, 2025 16:42

pr-processor bot added the Needs re-review label Mar 21, 2025

ehelms force-pushed the add-loadbalancer-support branch 2 times, most recently from 9363a87 to f35e485 Compare March 21, 2025 20:02

Fixes #38304 - Add loadbalancer support

57be1e3

Signed-off-by: Eric D. Helms <ericdhelms@gmail.com>

ehelms force-pushed the add-loadbalancer-support branch from f35e485 to 57be1e3 Compare March 21, 2025 20:22

pr-processor bot removed the Waiting on contributor label Mar 21, 2025

ianballou reviewed Mar 21, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Mar 21, 2025

ianballou approved these changes Apr 2, 2025

View reviewed changes

ehelms mentioned this pull request Apr 3, 2025

Add client_endpoint support to container_gateway plugin theforeman/puppet-foreman_proxy#863

Merged

ianballou merged commit 3db73cf into Katello:main Apr 4, 2025
11 checks passed

ianballou mentioned this pull request Apr 4, 2025

Bump version to 3.3.0 #51

Merged

ianballou mentioned this pull request May 1, 2025

Add container content note to load balancing guide theforeman/foreman-documentation#3824

Merged

10 tasks

	**settings.slice(:pulp_endpoint, :pulp_client_ssl_ca, :pulp_client_ssl_cert, :pulp_client_ssl_key, :loadbalancer)
	**settings.slice(:pulp_endpoint, :pulp_client_ssl_ca, :pulp_client_ssl_cert, :pulp_client_ssl_key, :client_content_endpoint)

Conversation

ehelms commented Jan 9, 2025

Uh oh!

ekohl commented Feb 28, 2025

Uh oh!

ianballou Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

ianballou Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

ianballou commented Mar 3, 2025

Uh oh!

ekohl commented Mar 3, 2025

Uh oh!

ianballou commented Mar 5, 2025

Uh oh!

ehelms commented Mar 7, 2025

Uh oh!

ekohl commented Mar 7, 2025

Uh oh!

ehelms commented Mar 10, 2025

Uh oh!

ianballou commented Mar 13, 2025

Uh oh!

ehelms commented Mar 13, 2025

Uh oh!

ehelms commented Mar 20, 2025

Uh oh!

ianballou commented Mar 20, 2025

Uh oh!

ianballou commented Mar 21, 2025

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants